[Core] Base python solver #2068

philbucher · 2018-05-14T16:16:40Z

This PR adds a baseclass for the python solvers to the core. This is the second step after having the AnalysisStage now
I used the functions that are currently used in the FluidDynamics and the StructuralMechanics Solver and put them to the base-class

This PR is to discuss the interface, I think/hope that it will not be so difficult this time since I am not really doing new things and only collected what exists already

After an agreement has been reached I will add documentation and finish integrating it in the Structural Solver

Having this base class will allow us to call all the functions in the interface in the stage without having to check if a solver actually implements them

Note that I didn't include Solve since after @RiccardoRossi s explanation we should call the functions separately
Also I did not include ImportModelPart, this one is splitted into ReadModelPart and PrepareModelPartForSolver
The first one is common for the solvers and will not have to be overwritten in most derived solvers.
The second is specific to the solvers and does what the name suggests
This separation is needed one one hand to have the reading implemented only once and also in case the Solver should not read the modelpart (used by the optimization guys)

loumalouomega · 2018-05-14T16:20:03Z

kratos/python_scripts/python_solver.py

+    def ComputeDeltaTime(self):
+        pass
+
+    def Initialize(self):


We can agree that the strategies will called in this cases, and then we can call this methods on the strategy

I intentionally left this empty since theoretically a solver can have no strategy

e.g. an FSI solver does not have a strategy itself

what do you mean that it cannot have a strategy inside? it may even have multiple as long as they are hidden in the api...

it can have 0 to n strategies inside, that why I left it empty

sorry i misread what you wrote.

loumalouomega · 2018-05-14T16:20:26Z

kratos/python_scripts/python_solver.py

+    def AdvanceInTime(self):
+        pass
+
+    def ComputeDeltaTime(self):


We can return the current delta time

ok, good suggestion, esp for the structure since it cannot compute Dt yet

Do we need this function on the external interface of the base class? I don't think it is used outside of the solver right now, and we could think of solvers that don't have it.

Also true
Will remove it, the user should call AdvanceInTime

loumalouomega · 2018-05-14T16:20:59Z

kratos/python_scripts/python_solver.py

+    def AddDofs(self):
+        pass
+
+    def GetMinimumBufferSize(self):


We can return 2 at least

As a default, the minimum buffer size. Obviously the idea is to override in the solvers

jcotela · 2018-05-14T16:29:34Z

kratos/python_scripts/python_solver.py

+
+    def ReadModelPart(self):
+        # TODO replace the functions in the solvers in Fluid and Structure
+        KratosMultiphysics.Logger.PrintInfo("::[PythonSolver]::", "Reading model part.")


One thing I was wondering is if we should "hard-code" labels like this. If we call it from a derived solver, is it more useful for the label to be "[PythonSolver]" or "[DynamicStructuralSolver]"? I don't have a strong opinion on this, but it is a discussion I think we should have at some point.

I thing, I remember or something... That in my analysis I added a method to return the label, that is overrided in each solver, so the solver always knows the name of the solver

It would be easy to add a GetSolverName function that every solver implements

Might be useful if several solvers are used, which is likely to be the case in the future

I dont habe a strong opinion though

Another point for hardcoding is that if a solver calls the functions of the baseclass (e.g. Initialize) then we would have multiple Initializing done prints with the same solver name which I think would be quite confusing

jcotela · 2018-05-14T16:31:07Z

kratos/python_scripts/python_solver.py

+        KratosMultiphysics.Logger.PrintInfo("ModelPart", self.main_model_part)
+        KratosMultiphysics.Logger.PrintInfo("::[PythonSolver]:: ", "Finished reading model part.")
+
+    def PrepareModelPartForSolver(self):


Again, does this need to be a public method of the solver? Or should it work internally? Also, the name is a bit strange... If we are "the solver", why should we prepare something "for the solver"?

I think it is good it is a public method, since one may want to override it. an example is if you have a "strange" problem and you need to build submodelparts on your own

I think it should be public
For fluids this is the function that would replace the elements, whereas in structure we e.g. assign the materials

RiccardoRossi

i am making minor comments.

another one is that
def validate_and_transfer_matching_settings

i guess should be done in c++ with its own tests etc

RiccardoRossi · 2018-05-15T07:11:22Z

kratos/python_scripts/python_solver.py

+    def AddDofs(self):
+        pass
+
+    def GetMinimumBufferSize(self):


for a static solver, 1 is enough...

RiccardoRossi · 2018-05-15T07:13:27Z

kratos/python_scripts/python_solver.py

+        KratosMultiphysics.Logger.PrintInfo("ModelPart", self.main_model_part)
+        KratosMultiphysics.Logger.PrintInfo("::[PythonSolver]:: ", "Finished reading model part.")
+
+    def PrepareModelPartForSolver(self):


I think it is good it is a public method, since one may want to override it. an example is if you have a "strange" problem and you need to build submodelparts on your own

RiccardoRossi · 2018-05-15T07:14:03Z

kratos/python_scripts/python_solver.py

+    def GetMinimumBufferSize(self):
+        pass
+
+    def ReadModelPart(self):


what if we call this ImportModelPart, exactly with the implementation and you propose...that is, also followed by a function "Prepare"

Fine for me

I used a different name because others (e.g. the FSI solvers) are still using ImportModelPart and I would have silently broken those

this argument makes sense.

RiccardoRossi · 2018-05-15T07:15:29Z

kratos/python_scripts/python_solver.py

+    def ComputeDeltaTime(self):
+        pass
+
+    def Initialize(self):


what do you mean that it cannot have a strategy inside? it may even have multiple as long as they are hidden in the api...

RiccardoRossi · 2018-05-15T07:16:20Z

kratos/python_scripts/python_solver.py

+    def Clear(self):
+        pass
+
+    def GetComputingModelPart(self):


Just to say that i invented this ComputingModelPart idea...and i hate it :-(.

I don't have a better idea however

philbucher · 2018-05-16T18:20:29Z

I finished the base-class and the adaption of the Structural Solvers
=> ready to be properly reviewed

I ran the tests for the involved applications

@jcotela I tried to do it also for the Fluid but there is a bit of refactoring needed, so I would prefer if it would be done in a separate PR

jcotela · 2018-05-17T08:33:50Z

@philbucher I'll make a pass of refactoring for fluid solvers once this is merged.

philbucher · 2018-05-19T14:29:29Z

ping @KratosMultiphysics/technical-committee

philbucher · 2018-05-24T10:04:25Z

ping @KratosMultiphysics/technical-committee I added the wiki for the AnalysisStage and the PythonSolver

Please have a look such that we can proceed with this and #2135

pooyan-dadvand · 2018-05-24T15:11:26Z

kratos/python_scripts/python_solver.py

+        """
+        pass
+
+    def Check(self):


I would add some trivial checks (like settings is a Parameter, input file exists, etc.)

the type of the input is checked in the constructor

I thought of this function to be called before the SolutionLoop (I will add it to the AnalysisStage)
=> It should call methods like Strategy.Check() to validate if the input is correct

Therefore it cannot validate if the input file has been found
(btw for this one the error is quite clear already:
RuntimeError: Error: Error opening mdpa file : Example.mdpa)

philbucher · 2018-05-25T13:57:35Z

I did the modifications as requested by @RiccardoRossi => passing the Model instead of the ModelPart to the solver

@KratosMultiphysics/technical-committee please have a look since this has to be merged before #2135

After this is merged I will solve the "conflicts" in #2135 to finalize everything

loumalouomega · 2018-05-26T15:20:12Z

@philbucher feel free to merge both #2135 and this PR

RiccardoRossi

i am approving. @philbucher wait if someone blocks it tomorrow in the morning and otherwise merge...

RiccardoRossi · 2018-05-19T18:46:12Z

kratos/python_scripts/python_solver.py

@@ -95,7 +95,7 @@ def ExportModelPart(self):
        file.close()
        KratosMultiphysics.ModelPartIO(name_out_file, KratosMultiphysics.IO.WRITE).WriteModelPart(self.main_model_part)

-    def AdvanceInTime(self):
+    def AdvanceInTime(self, current_time):


current time can be obtained as

model_part.ProcessInfo[TIME]

why do you need to pass it?

loumalouomega · 2018-05-28T08:39:51Z

@philbucher I have been doing experiments with a branch, merging this PR and #2135, and for example the fluid does not work anymore, @jcotela do you mind if I repair that?

loumalouomega · 2018-05-28T08:42:49Z

kratos/python_scripts/python_solver.py

+        """
+        raise Exception('Please implement "GetMinimumBufferSize" in your derived solver')
+
+    def ImportModelPart(self):


There are two of these, there is the ImportModelPart and the _ImportModelPart, why?

OK, deprecated, I see

philbucher · 2018-05-28T09:25:00Z

@loumalouomega please wait with pushing to this Branch, I am fixing FSI and your branch before merging this

@jcotela and I know abt the problems, the fluid will be done in a separate PR before #2135

Please give me until this evening then everything should be working

loumalouomega · 2018-05-28T09:57:31Z

Don't worry, I am working locally, but right no I am havig problems to make work the model properly with the main model part

philbucher · 2018-05-28T10:01:11Z

ok, but be aware that I am also working on this

I wil push the changes regarding the solver to this branch in ~2hrs and afterwards will integrate the changes in the AnalyisStage-Pr

…ration og #2135

philbucher · 2018-05-28T11:42:46Z

this is the final set of modifications
@loumalouomega @rubenzorrilla I updated your apps, the tests that run on master are still working

@KratosMultiphysics/technical-committee there were some minor things I still had to fix, in relation with #2135 , but I didn't change anything major

I will merge this branch now to #2135 to fully apply the changes there

@RiccardoRossi switching to the Model (=> #2137 ) should now only be a minor change

loumalouomega

OK

philbucher · 2018-05-28T15:04:05Z

I will merge this tmr morning if none blocks/complains

rubenzorrilla · 2018-05-29T08:19:11Z

Apart of the fact that I did not have time to check it until now, this breaks retro compatibility (at least for the FSI app). I guess that this is something we have to live with.

In any case, it is not compatible with the current Main script generated by the interface.

philbucher added 2 commits May 14, 2018 18:03

Adding first version of solver

7335fd4

Applying some of the changes to the StructMech solver

8c58792

philbucher added Enhancement Kratos Core Applications Python labels May 14, 2018

philbucher self-assigned this May 14, 2018

philbucher requested a review from a team May 14, 2018 16:16

loumalouomega requested changes May 14, 2018

View reviewed changes

jcotela reviewed May 14, 2018

View reviewed changes

RiccardoRossi reviewed May 15, 2018

View reviewed changes

philbucher added 7 commits May 16, 2018 13:48

WIP

16a02cf

Properly integrated Restart

5d05fe4

Adapting Structural Solver

d032242

Merge branch 'master' into core/base-solver

c2aa2b1

Final Fixes in StructuralMechanics

f1f890e

More fixes for consistency

08e41df

removing comment

5b7bf48

codacy suggestion

fb8dab0

philbucher mentioned this pull request May 20, 2018

[Core] update analysis stage #2135

Merged

philbucher added 2 commits May 24, 2018 12:00

Merge branch 'master' into core/base-solver

d77534b

minor

636ab02

pooyan-dadvand reviewed May 24, 2018

View reviewed changes

jcotela mentioned this pull request May 24, 2018

Fluid/feature adjoint analysis #2183

Merged

Passing the Model to the solver insted of ModelPart

0b50e99

loumalouomega previously approved these changes May 26, 2018

View reviewed changes

loumalouomega mentioned this pull request May 27, 2018

Model as a static of kernel #2137

Closed

RiccardoRossi previously approved these changes May 27, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into core/base-solver

438a0af

loumalouomega reviewed May 28, 2018

View reviewed changes

philbucher added 6 commits May 28, 2018 12:48

ReadModelPart => ImportModelPart

05b1c45

Changes to fully support passing the Model to the solver and in prepa…

b4a8ef1

…ration og #2135

FSI: Adapting Emulator Test

71ef4de

FSI: Adapting Solvers

dd8a542

Contact: Adapting Analysis and solver_wrapper

44675e5

Maving the time settings to solver as preparation of #2135

e97f752

philbucher dismissed stale reviews from RiccardoRossi and loumalouomega via e97f752 May 28, 2018 11:37

loumalouomega approved these changes May 28, 2018

View reviewed changes

loumalouomega added a commit that referenced this pull request May 28, 2018

Merge #2068

0dcb323

philbucher mentioned this pull request May 28, 2018

Fix/structural response for analysis stage #2171

Merged

philbucher merged commit cfce65b into master May 29, 2018

philbucher deleted the core/base-solver branch May 29, 2018 05:01

rubenzorrilla mentioned this pull request May 29, 2018

Removing structure model part from FSI solvers constructors #2211

Merged

jcotela mentioned this pull request Jun 4, 2018

Fluid/feature python solver derivation #2262

Merged

[Core] Base python solver #2068

[Core] Base python solver #2068

Conversation

philbucher commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcotela May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiccardoRossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbucher commented May 16, 2018

jcotela commented May 17, 2018

philbucher commented May 19, 2018

philbucher commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbucher commented May 25, 2018 • edited Loading

loumalouomega commented May 26, 2018

RiccardoRossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loumalouomega commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbucher commented May 28, 2018

loumalouomega commented May 28, 2018

philbucher commented May 28, 2018

philbucher commented May 28, 2018

loumalouomega left a comment

Choose a reason for hiding this comment

philbucher commented May 28, 2018

rubenzorrilla commented May 29, 2018

jcotela May 14, 2018 •

edited

Loading

philbucher commented May 25, 2018 •

edited

Loading